Skip to content

fix: detect primary keys in PostgreSQL, Redshift, MSSQL, and ClickHouse drivers#254

Merged
datlechin merged 2 commits intomainfrom
fix/primary-key-detection-drivers
Mar 10, 2026
Merged

fix: detect primary keys in PostgreSQL, Redshift, MSSQL, and ClickHouse drivers#254
datlechin merged 2 commits intomainfrom
fix/primary-key-detection-drivers

Conversation

@datlechin
Copy link
Copy Markdown
Collaborator

@datlechin datlechin commented Mar 10, 2026

Summary

  • Fix isPrimaryKey always returning false in PostgreSQL, Redshift, MSSQL, and ClickHouse fetchColumns/fetchAllColumns methods, which caused DELETE and UPDATE queries to use all columns in the WHERE clause instead of just the primary key
  • PostgreSQL/Redshift: add LEFT JOIN on information_schema.table_constraints + key_column_usage to detect PK columns in both fetchColumns and fetchAllColumns
  • MSSQL: same LEFT JOIN pattern in fetchColumns (uses default fetchAllColumns)
  • ClickHouse: pre-fetch PK info from system.tables into a lookup dictionary in fetchAllColumns (fetchColumns already worked)
  • Add 5 regression tests verifying PK-only WHERE clauses for each affected database type

Test plan

  • All 151+ existing SQLStatementGenerator tests pass
  • 5 new SQLStatementGeneratorPKRegressionTests pass
  • Manual: connect to PostgreSQL, open table with PK, delete row → verify DELETE uses PK-only WHERE
  • Manual: connect to MSSQL, open table with PK, delete row → verify DELETE uses PK-only WHERE
  • Manual: connect to ClickHouse, open table with PK, delete row → verify DELETE uses PK-only WHERE

Summary by CodeRabbit

  • Bug Fixes

    • Fixed DELETE and UPDATE query generation for PostgreSQL, Redshift, MSSQL, and ClickHouse.
    • Corrected column metadata so primary-key status is reported accurately across supported databases.
  • Tests

    • Added regression tests validating DELETE and UPDATE behaviors and primary-key-aware SQL generation across database types.

@chatgpt-codex-connector
Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 10, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: fa48406d-0cf1-4cb9-b591-22181e1a47c8

📥 Commits

Reviewing files that changed from the base of the PR and between 9702121 and fb2df4b.

📒 Files selected for processing (4)
  • Plugins/ClickHouseDriverPlugin/ClickHousePlugin.swift
  • Plugins/PostgreSQLDriverPlugin/PostgreSQLPluginDriver.swift
  • Plugins/PostgreSQLDriverPlugin/RedshiftPluginDriver.swift
  • TableProTests/Core/ChangeTracking/SQLStatementGeneratorPKRegressionTests.swift

📝 Walkthrough

Walkthrough

Adds primary-key detection to multiple database drivers so column metadata marks actual PK columns; DELETE and UPDATE generation now include all PK columns in WHERE clauses. A changelog entry and regression tests covering PostgreSQL, Redshift, MSSQL, and ClickHouse were added.

Changes

Cohort / File(s) Summary
Changelog
CHANGELOG.md
Adds a Fixed entry noting DELETE and UPDATE now use all PK columns in WHERE clauses for PostgreSQL, Redshift, MSSQL, and ClickHouse.
PostgreSQL & Redshift drivers
Plugins/PostgreSQLDriverPlugin/PostgreSQLPluginDriver.swift, Plugins/PostgreSQLDriverPlugin/RedshiftPluginDriver.swift
Queries updated to include an is_pk indicator via PK-derived subqueries; parsing reads is_pk and sets PluginColumnInfo.isPrimaryKey accordingly for per-table and all-columns paths.
MSSQL driver
Plugins/MSSQLDriverPlugin/MSSQLPlugin.swift
Column query now LEFT JOINs a PK subquery and returns IS_PK; parsing reads the new field and sets isPrimaryKey from it instead of hardcoding false.
ClickHouse driver
Plugins/ClickHouseDriverPlugin/ClickHousePlugin.swift
Adds an upfront PK lookup from system.tables to build per-table key column sets and uses that mapping to mark isPrimaryKey when assembling PluginColumnInfo.
Tests
TableProTests/Core/ChangeTracking/SQLStatementGeneratorPKRegressionTests.swift
New regression test suite validating DELETE and UPDATE SQL generation uses PK-only WHERE clauses across PostgreSQL, MSSQL, ClickHouse, and Redshift, including batch/delete variants and placeholder checks.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 I dug through rows, found keys that hide,

I stitched WHERE clauses with them side by side.
Four drivers hum, the tests all cheer,
DELETEs and UPDATEs now know who's dear.
Hoppity hops — the fixes are here! 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main change: detecting primary keys across four database drivers, which directly addresses the root cause of the DELETE/UPDATE WHERE clause bug.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/primary-key-detection-drivers

Comment @coderabbitai help to get the list of available commands and usage tips.

@datlechin datlechin merged commit c87a6af into main Mar 10, 2026
3 checks passed
@datlechin datlechin deleted the fix/primary-key-detection-drivers branch March 10, 2026 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant